-
Notifications
You must be signed in to change notification settings - Fork 842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow EuiContextMenuPanels to update when their children changes #710
Allow EuiContextMenuPanels to update when their children changes #710
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎸 THANK YOU FOR ADDING TESTS! This solution looks great. Really nice analysis in the originating issue. I just had a couple comments for your consideration.
} | ||
} | ||
|
||
// if this panel was given any children, return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment really adds much more information than the code already provides, because it's describing what the code does instead of why it does it. I think we can make this comment more helpful by explaining the intention here, like you did in the PR description, e.g.
// Allow children to re-render.
describe('updating items and content', () => { | ||
describe('updates to items', () => { | ||
it(`should not re-render if any items's watchedItemProps did not change`, () => { | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this Promise? I tested it without it and the test seemed to work normally. Maybe I'm missing something?
Also, I found this test a little difficult to follow because of the indirection added by getItem
and makeItems
. I think if we hard-coded the items we're passing in then this code will be become more direct (and thereby more readable, at least to me 😅). It becomes a little more difficult to change, but I think this code will be read much much more often than its read, mostly because these parts of the tests probably won't change for a long time. WDYT?
it(`should not re-render if any items's watchedItemProps did not change`, () => {
const items = [(
<EuiContextMenuItem key="A" data-counter={0}>
Option A
</EuiContextMenuItem>
), (
<EuiContextMenuItem key="B" data-counter={1}>
Option B
</EuiContextMenuItem>
)]
// by not passing `watchedItemProps` no changes to items should cause a re-render
const component = mount(
<EuiContextMenuPanel
items={items}
/>
);
expect(component.debug()).toMatchSnapshot();
component.setProps(
{ items: makeItems() },
() => {
expect(component.debug()).toMatchSnapshot();
}
);
});
}); | ||
|
||
it(`should re-render if any items's watchedItemProps did change`, () => { | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment applies here and to the other test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM! Just had a question about whether setProps
is async.
expect(component.debug()).toMatchSnapshot(); | ||
} | ||
); | ||
expect(component.debug()).toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention this before, you can import takeMountedSnapshot
from the test
module on line 4, and use it here like this: expect(takeMountedSnapshot(component)).toMatchSnapshot();
, which will yield a snapshot of regular HTML:
<div
class="euiContextMenuPanel"
tabindex="0"
>
<div>
<button
class="euiContextMenuItem"
data-counter="0"
type="button"
>
<span
class="euiContextMenu__itemLayout"
>
<span
class="euiContextMenuItem__text"
>
Option A
</span>
</span>
</button>
<button
class="euiContextMenuItem"
data-counter="1"
type="button"
>
<span
class="euiContextMenu__itemLayout"
>
<span
class="euiContextMenuItem__text"
>
Option B
</span>
</span>
</button>
</div>
</div>
It's definitely not necessary to use this in these tests, but I thought I should mention it!
items={makeItems()} | ||
/> | ||
); | ||
expect.assertions(2); // make sure the assertion in the `setProps` callback is executed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests still execute both assertions if this line is removed. Is this a defensive measure in situations where setProps
is asynchronous? I did some digging to see if this was the case but Enzyme's docs don't mention anything about sync vs async, so this is new ground for me.
jenkins test this |
71b6d00
to
f76b5cc
Compare
jenkins test this |
Fixes #674.
EuiContextMenu
accepts panel shapes with eitheritems
orcontent
, passing that data through toEuiContextMenuPanel
. The panel component'sshouldComponentUpdate
previously only checked if it was in a transitioning state or if its items had changed, and had no logic to allow it'schildren
(theEuiContextMenu
'scontent
) to update.This PR changes
EuiContextMenuPanel
'sshouldComponentUpdate
to returntrue
if any children were passed. This pushes a responsibility onto that child content to self-manage if/when there are performance considerations when rendering that content (which is better practice anyway).